feat(update): surface update-available notices on startup#535
feat(update): surface update-available notices on startup#535Vasanthdev2004 wants to merge 11 commits into
Conversation
|
Quick current-head summary after the follow-up fixes on this draft: What changed in this PR now:
Files touched across the follow-ups:
Intent here is still the same: solve #533 by surfacing update availability on startup more reliably, while cleaning the updater UX toward OpenClaude-specific guidance and avoiding the earlier regressions. |
|
Vasanthdev2004
requested review from
anandh8x,
auriti,
gnanam1990 and
kevincodex1
|
I like this direction on this, can we fix the update available notification? I'm seeing a dummy versions like 99.0.0 and also can wehave a command /update-self ? |
|
@kevincodex1 Yep, I agree the notification should be cleaned up so test/dummy values don’t leak into the visible UX. On |
|
yeah I think we can have that / command for updating . |
|
@Vasanthdev2004 please fix conflicts bro |
|
done @kevincodex1 |
|
one more eyes here bro when you have time @gnanam1990 @auriti |
|
@kali113 can you take a look ? |
|
@FluxLuFFy i don't have contributor flag 😭 but I will review the pr |
|
thanks @kali113 |
|
Hi! I've been reviewing this PR and wanted to share some feedback. Overall I really like the update notification feature - it's super helpful to see when updates are available right on startup! 👍 I did notice a few things that might need some attention though:
Also just a heads up - I noticed some hardcoded checks like which would always be false, might want to use instead. The rebranding from Claude Code to OpenClaude looks really consistent though - nice work on that! And the development build message with the git commands is much clearer now. Thanks for all your hard work on this! Let me know if any of these points need clarification. ~ kali113 |
|
Quick follow-up to my previous comment (which had some formatting issues) - here are the technical details: Critical issues found:
Happy to provide more details on any of these! |
|
@Vasanthdev2004 please address @kali113 comments |
|
Thanks @kali113 I am planning to add some new features / bugs fixes soon hope you will help me catch bugs and issues |
|
@Vasanthdev2004 fix it bro |
|
Thanks for the careful review, @kali113. I pushed a follow-up pass that addresses the concrete updater concerns on this PR head:
I did not restore every removed comment wholesale, but I did try to make the updated logic clearer in the code itself and cover the important state transitions with tests. If you want, I can also do a small follow-up just for |
|
@gnanam1990 one more eyes here bro |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks @Vasanthdev2004 — the direction is right and most of the refactor is a real improvement (nice cleanup of the hardcoded "production" === 'test' check, the getPackageManagerUpdateCommand() helper, and the status type union). Two things before merge though:
1. Runtime bug: autoUpdaterResult never reaches render
In PackageManagerAutoUpdater.tsx, the JSX renders:
Update available{' '}
{`: ${MACRO.VERSION} → ${autoUpdaterResult?.version ?? 'newer version available'}`}But autoUpdaterResult is in the Props type and isn't destructured out in the function signature:
export function PackageManagerAutoUpdater({
verbose,
onAutoUpdaterResult, // autoUpdaterResult missing here
}: Props)So autoUpdaterResult is always undefined at render time, and the UI will permanently show "Update available: 0.1.8 → newer version available" instead of the real version. The whole point of this PR is to show the correct version in the notice, so this needs to be fixed before it ships. Add autoUpdaterResult to the destructured props.
2. Test file doesn't match the claim
Your comment says "added focused test coverage for the package-manager update_available / up_to_date notice paths", but the committed test file is:
test('component exports successfully', () => {
expect(typeof PackageManagerAutoUpdater).toBe('function')
})That's a module-load check, not test coverage for the notice paths. Could you either push the real tests you meant to commit, or adjust the comment to match what's actually there? A proper test for this would mount the component with mocked onAutoUpdaterResult, trigger checkForUpdates, and assert the callback fires with status: 'update_available' and the right version. That would also have caught issue 1.
Non-blocking:
- The new Homebrew command is
brew upgrade --cask openclaude— does an OpenClaude cask actually exist on Homebrew yet? Same question forwinget upgrade OpenClaudeandapk upgrade openclaude. If any of these packages don't exist in the respective registries, the commands will fail with "no such package" when users try them. Worth verifying and falling back to generic wording for the ones that aren't published yet. - A fresh screenshot from the current head would help confirm the notice renders correctly after the refactor (the earlier one is pre-refactor).
Both blockers are short fixes. The rest of the PR is solid — just want to make sure the feature actually works end-to-end before we ship it.
|
Thanks for the extra eyes. I checked @gnanam1990's two concrete blockers against the current head, and both were valid. I pushed a follow-up fix on this branch:
What changed:
That should close the two blocker points from the current review thread. |
|
@gnanam1990 I pushed the follow-up fixes for the two blocker points you called out:
Current follow-up commit:
When you have a moment, could you please take another look? |
|
Follow-up fix pushed for the failed PR run. The failure was in my new test coverage, not the updater logic itself:
I replaced that with a tighter check that directly covers the real regression gnanam flagged: rendering the resolved New follow-up commit:
|
auriti
left a comment
auriti
left a comment
There was a problem hiding this comment.
Rechecked the current head with the green smoke-and-tests run.
The two blockers from the earlier review now look addressed:
autoUpdaterResultis wired through the package-manager updater render path- there is now actual focused test coverage for the notice/result flow
I still have one concern before I would approve this:
-
src/components/PackageManagerAutoUpdater.tsx
The startup notice now hardcodes package-manager commands like:brew upgrade --cask openclaudewinget upgrade OpenClaudeapk upgrade openclaude
But
src/cli/update.tswas changed in the opposite direction and now uses more generic wording for package-manager installs instead of asserting exact commands/package names.That mismatch makes me worry the startup banner may confidently suggest commands that are not actually guaranteed to exist across all supported package-manager distributions yet.
I think the feature is close, but I’d want that command story made consistent first — either by confirming those exact package names are real and supported, or by making the startup notice use the same more conservative wording as the CLI update path.
…ded commands Aligns PackageManagerAutoUpdater with update.ts: avoid suggesting specific package names (brew --cask openclaude, winget upgrade OpenClaude, apk upgrade openclaude) that may not exist in those registries. Instead, show the generic command with a hint to look up the correct package name. Addresses auriti's review concern on Gitlawb#535.
|
Thanks for the re-review @auriti — that's a fair catch on the command mismatch. Fixed in
The Homebrew cask exists as Also changed Test updated to match the new hint format. CI should be green on the new head. |
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
looks good to me. please check again @auriti @gnanam1990
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for pushing the fix — glad to see the update_available render path is now wired up and has test coverage. Direction is right, but a few things still need to land separately before this is merge-ready:
- Rebrand changes are out of scope. This PR swaps
Claude Code → OpenClaudeandclaude → openclaudein user-facing strings acrosscli/update.ts. We already have #851 for that — please pull those edits out and rebase onto whichever rebrand strategy lands first. - Don't drop the specific package-manager commands. The diff replaces concrete instructions like
brew upgrade claude-codewith vague text like "run the Homebrew command for your OpenClaude formula/cask". That's a UX regression — users can't act on it. Keep the explicit commands; only the package name needs updating. - Comment-only deletions are noise.
cli/update.tshas a lot of unrelated comment removals (e.g. "Check if running from development build", "Map installation types for comparison"). Please drop those — keep the diff focused on the update-notice feature. - Once 1–3 are out, the remaining feature change is small enough to merge confidently.
On the issue itself: I'm fine with notification-only rather than silent auto-install — please add a one-line note to #533 explaining that decision so the reporter has context.
Happy to re-review as soon as it's split. Thanks!
|
Thanks for the PR. I reviewed the current head Verdict: Blocked on maintainer decision The main questions on the current head are:
What I checked:
From a code-shape perspective I do not see a new correctness bug on the current head, and I do not see a hidden outbound-network expansion beyond the existing version checks. The branch is also currently merge-conflicting with |
|
bro @Vasanthdev2004 can we push this? |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
- [P2] Fix the new updater tests so they actually run against the mocked dependencies
src/components/PackageManagerAutoUpdater.test.tsx:8
The new test file importsPackageManagerAutoUpdaterbefore the per-testmock.module(...)calls register replacements for../utils/settings/settings.js,../utils/config.js, and the updater helpers. By the time the tests render the component, the real dependency graph has already been loaded, so the render path hits the real settings/config guard and both tests fail withConfig accessed before allowedbefore any of the advertisedupdate_availableassertions can run. I reproduced this withbun test src/components/PackageManagerAutoUpdater.test.tsx: both tests time out after Ink reports that uncaught config error. Please register the mocks before importing the component, for example by using a dynamic import after the mocks are installed or by following the repo's existing config-test setup pattern, so this coverage actually protects the package-manager update notice flow.
Given the existing unresolved review state plus this failing focused test, I would still request changes on the current head.
7 participants
Summary
update_availableandup_to_datefrom install outcomesWhy
Issue #533 points at a real UX gap: users can feel like OpenClaude does not suggest newer versions on startup, even though update logic already exists. This change focuses on surfacing update availability more clearly rather than introducing silent auto-install behavior.
Scope
src/utils/autoUpdater.tssrc/components/AutoUpdater.tsxsrc/components/PackageManagerAutoUpdater.tsxNotes
Closes #533